Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add filesystem store backend #251

Open
wants to merge 9 commits into
base: master
Choose a base branch
from
Open

Add filesystem store backend #251

wants to merge 9 commits into from

Conversation

geschke
Copy link

@geschke geschke commented Nov 28, 2023

When testing the sessions middleware, I realized that a filesystem store was missing, which is the default in PHP. Upon examining the underlying Gorilla sessions library, I discovered an experimental implementation of a filesystem store. Despite its experimental status, it performed well in my tests when integrated into a Gin application using pure Gorilla sessions.

To support the Gorilla FilesystemStore, I added the necessary code in a filesystem backend module, provided by this pull request.

While testing the filesystem module, I noticed discrepancies in the test files for the backend modules. For instance, the cookie_test.go file didn't work as expected - the function "NewStore()" was missing. After modifying it to "cookie.NewStore()" and importing the "gin-contrib/sessions/cookie" module, the test ran successfully with the command "go test cookie/cookie_test.go". I didn't make changes to another module in this pull request, since I'm not very experienced with the Golang test system. Please correct me if I'm mistaken.

In the "filesystem_test.go" file, which is essentially a modified copy of the cookie test, I used the function "filesystem.NewStore()", and the tests ran successfully in my environment. However, the test revealed an issue with the Options settings in the FilesystemStore of the Gorilla sessions module version 1.2.1, used in this library. This problem has been resolved in the current version, 1.2.2, so I updated the version number in go.mod.

I would appreciate it if you could merge this pull request, as I believe the addition of the filesystem store is valuable, particularly for development and testing purposes.

Thanks in advance &
Kind regards,
Ralf

@manusa
Copy link

manusa commented Mar 5, 2024 •
edited
Loading

I just came into this PR because I implemented the store based on gorilla/sessions FilesystemStore (identically 😳) in a downstream project and was looking to contribute it back upstream.

Is this PR (should fix #182) being considered?

@appleboy
Copy link
Member

@geschke Please help to take a look build fails.

@geschke
Copy link
Author

geschke commented Mar 25, 2024

Perhaps I misunderstood how the test file is meant to be applied. I attempted to run the tests locally, but encountered errors when using the commands specified in the testing.yml file. It appears that the issue lies in the import of the 'filesystem' package. Currently, the initial lines of filesystem_test.go are as follows

package filesystem

import (
	"os"
	"testing"

	"github.com/gin-contrib/sessions"
	"github.com/gin-contrib/sessions/filesystem"
	"github.com/gin-contrib/sessions/tester"
)

var sessionPath = os.TempDir()

var newStore = func(_ *testing.T) sessions.Store {
	store := filesystem.NewStore(sessionPath, []byte("secret"))
	return store
}
[...]

This works fine when I run go test filesystem/filesystem_test.go locally:

geschke@moabit:~/go/src/gin-contrib-sessions$ go test filesystem/filesystem_test.go
ok      command-line-arguments  0.025s

But it fails with the go test command in testing.yml.

When I remove the filesystem import and use the Newstore() function without the package name, the local test build fails. However, it's possible that it might work in the GitHub runner.

package filesystem

import (
	"os"
	"testing"

	"github.com/gin-contrib/sessions"
	//"github.com/gin-contrib/sessions/filesystem"
	"github.com/gin-contrib/sessions/tester"
)

var sessionPath = os.TempDir()

var newStore = func(_ *testing.T) sessions.Store {
	store := NewStore(sessionPath, []byte("secret"))
	return store
}

Please provide guidance on how to proceed. Should I commit the mentioned changes? Unfortunately, I seem unable to run the tests in my GitHub repository; perhaps I've overlooked something here.

Thanks in advance &
kind regards,
Ralf

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@geschke @manusa @appleboy